Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standalone 3/N: Define the Zarr logger #295

Merged
merged 20 commits into from
Sep 26, 2024
Merged

Standalone 3/N: Define the Zarr logger #295

merged 20 commits into from
Sep 26, 2024

Conversation

aliddell
Copy link
Member

@aliddell aliddell commented Sep 17, 2024

Depends on #294.

@aliddell aliddell changed the title Define the Zarr logger Standalone 3/N: Define the Zarr logger Sep 17, 2024
Copy link
Collaborator

@shlomnissan shlomnissan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this PR looks good, but there are two issues I consider blockers:

  • Having underlying types that belong to one target defined in another creates potentially unnecessary coupling. While not critical, it's worth addressing.
  • More importantly, using std::localtime in a multi-threaded environment is likely a bug. We should double-check this before merging.

@@ -0,0 +1,25 @@
set(CMAKE_POSITION_INDEPENDENT_CODE ON)

set(tgt acquire-zarr-logger)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of nits and FYIs:

  • I believe this target name can be generalized to acquire-logger. It doesn't need to be Zarr-specific.
  • Directly using the target name instead of a variable improves readability, maintainability, and the overall robustness of your CMake configuration. The indirection of a variable is unnecessary in most use cases and can introduce subtle bugs or confusion if you have multiple targets. Most of the time people use it to reduce redundancy, but it's typically unnecessary.

@@ -0,0 +1,30 @@
#include "zarr.types.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that these types are not defined within the logger target compromises encapsulation. If the logger is defined as a standalone target, it should be self-contained and reusable. Could we create an additional header file, perhaps named logger_types.h, with a C interface to address this issue?

logger.cpp
)

set(PUBLIC_INCLUDE_DIR ${CMAKE_SOURCE_DIR}/include/)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is only used once, we could inline this variable.

...)
{
std::scoped_lock lock(log_mutex_);
if (current_level_ == ZarrLogLevel_None || level < current_level_) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This relates to the filtering issue discussed in the previous PR. My preferred API would allow users to enable or disable logs while handling filtering themselves. I find the current pattern suboptimal because it assumes users know our predefined log levels and lacks flexibility. For instance, a user might want to see info logs but not debug logs.

Comment on lines 38 to 39
va_list args;
va_start(args, format);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this in the comprehensive PR review, but a modern approach to handling an arbitrary number of arguments is using variadic templates.

Aspect va_list (C) Variadic Templates (C++)
Type Safety None, relies on the programmer to manage types Full type safety, checked by the compiler
Runtime vs Compile-time Arguments processed at runtime Arguments processed at compile time
Ease of Use Requires manual handling with macros Recursion or fold expressions make usage simpler
Flexibility Limited to basic types and needs type casting Supports any type (built-in and user-defined)
Error Handling Type errors lead to runtime crashes or UB Errors caught at compile-time
Performance Slower (runtime argument parsing) Faster (resolved at compile-time)
Common Use Cases C-style printf, legacy C functions Generic functions or classes in modern C++

Comment on lines 41 to 42
std::string prefix;
std::ostream* stream = &std::cout;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::string prefix;
std::ostream* stream = &std::cout;
auto prefix = std::string {};
auto& stream = &std::cout;

break;
case ZarrLogLevel_Warning:
prefix = "[WARNING] ";
stream = &std::cerr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would a warning message go to std::cerr?

Comment on lines 55 to 58
case ZarrLogLevel_Error:
prefix = "[ERROR] ";
stream = &std::cerr;
break;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add default case for completeness. Are we using a code linter like clangd/clang-tidy or sonar? This issue should be flagged by such tools.

Comment on lines +62 to +66
auto now = std::chrono::system_clock::now();
auto time = std::chrono::system_clock::to_time_t(now);
auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(
now.time_since_epoch()) %
1000;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I mentioned this in the large PR review. I would extract this code and std::put_time to a separate function. This snippet is copied from a logger that I wrote a while back so it may need to be adjusted a bit if you decide to use it.

auto GetTimestamp() -> std::string {
    using std::chrono::system_clock;
    auto now = system_clock::now();
    auto in_time_t = system_clock::to_time_t(now);

    auto time_info = std::tm{};
    #ifdef _WIN32
        localtime_s(&time_info, &in_time_t);
    #else
        localtime_r(&in_time_t, &time_info);
    #endif

    return fmt::format("{:04}-{:02}-{:02} {:02}:{:02}:{:02}",
                       time_info.tm_year + 1900,
                       time_info.tm_mon + 1,
                       time_info.tm_mday,
                       time_info.tm_hour,
                       time_info.tm_min,
                       time_info.tm_sec);
}

std::string filename = filepath.filename().string();

// Output timestamp, log level, filename
*stream << std::put_time(std::localtime(&time), "%Y-%m-%d %H:%M:%S") << '.'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that std::localtime is not thread-safe. See my previous code snippet that uses localtime_s or localtime_r.

Function Thread Safety Availability Usage
std::localtime Not thread-safe C++ standard (all platforms) Uses static struct tm, may cause data races in multi-threaded environments.
std::localtime_s Thread-safe Windows, C11 Caller provides struct tm. Part of Microsoft's C++ library, available on Windows.
std::localtime_r Thread-safe POSIX (Linux/macOS) Caller provides struct tm. Commonly used in Unix-like systems.

Base automatically changed from standalone-sequence-3 to main September 24, 2024 15:49
@aliddell aliddell force-pushed the standalone-sequence-4 branch from 977b8da to 39ec6a7 Compare September 24, 2024 19:47
Copy link
Collaborator

@shlomnissan shlomnissan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@aliddell aliddell merged commit 0658371 into main Sep 26, 2024
3 checks passed
@aliddell aliddell deleted the standalone-sequence-4 branch September 26, 2024 16:48
aliddell added a commit that referenced this pull request Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants